SonarCloud Quality Gate failed.   

1 Bug
0 Vulnerabilities
0 Security Hotspots
17 Code Smells

No Coverage information
0.0% Duplication

Preview of the CSS tooltip:

What others think about this solution?

Codecov Report

Base: 71.17% // Head: 71.17% // No change to project coverage 👍

Coverage data is based on head (9c70cdd) compared to base (dd998da).
Patch has no changes to coverable lines.

@@            Coverage Diff            @@
##             master    #1324   +/-   ##
=========================================
  Coverage     71.17%   71.17%           
  Complexity     1650     1650           
=========================================
  Files            32       32           
  Lines          3788     3788           
=========================================
  Hits           2696     2696           
  Misses         1092     1092           

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

This PR is a bit old, I think it would be best to merge the changes from master and possibly rebase as well

Hi @osma, replies inline

I agree 100% that a pure CSS approach is preferable to a JS-based solution such as Popper. This PR looks like a big improvement. Some comments:

Excellent!

• Status

1. I think the animation (appearing from the top) is unnecessary and just adds to visual noise. As a metaphor it doesn't make sense to me - why would the popup window appear from the top? If anything, I think it should "emerge" from the spot where the cursor is pointed, like smoke that comes out from a chimney :) But just appearing out of nowhere, like with qtip2, would be best.

Agreed. I actually prefer no animation if possible. Will change it.

• Status

2. There's a little problem with the cross that is used to clear the contents of the search box - it will stay on top of the popup (probably need to adjust z-index?). I can't see the cross at all in your animation but here's how it looks to me (similar in both Firefox and Chrome):

Oh, doesn't look good. Let me see what I can do to improve it. Good catch!

EDIT: Found it. You were correct about the z-index. The clear-search CSS class had z-index: 9001, and the tooltip had 555. Changed the tooltip z-index to 9002.

• Status

3. There are other places in the UI where qtip2 is used, you need to address those as well obviously (as pointed out in a comment)

Roger that 👍 I will update the other ones once we have fixed the feedback items above.

• Status

4. I think you also need to remove the following lines:

light.twig:<link href="resource/css/jquery.qtip.min.css" rel="stylesheet" type="text/css" />
scripts.twig:<script src="vendor/grimmlink/qtip2/dist/jquery.qtip.min.js"></script>

Ah! Yup. I left that because I didn't disable qtip in other places, I think. From what I remember, I left it enabled somewhere else and then opened two separate browser windows, to compare the generated CSS and how the UI behaved.

This will be removed when I have finished the item above, of updating all the other UI elements that use qtip 👍

Thanks!!

Ah! Yup. I left that because I didn't disable qtip in other places, I think. From what I remember, I left it enabled somewhere else and then opened two separate browser windows, to compare the generated CSS and how the UI behaved.

Looks like I'm wrong here. I actually removed the qtip from composer.json, so no reason for keeping these files. Let me remove them now.

Note to self:

1. Remove call to qtip, in docready.js;

$('#navi4').qtip(qtip_skosmos);

2. Find the original span for that qtip removed, in the views folder;

3. Add the CSS classes for new tooltip, class="skosmos-tooltip-wrapper skosmos-tooltip t-bottom";

4. Rename the title="…" to data-title="…";

5. Replace any <br/>'s by 
;

Example:

• Before:

<span id="navi4" tabindex="0" title="{% trans "helper_help" %}<br /><br />{% trans "search_example_text" %}">

• After:

<span class="skosmos-tooltip-wrapper skosmos-tooltip t-bottom" id="navi4" tabindex="0" data-title="{% trans "helper_help" %} &#xa; &#xa; {% trans "search_example_text" %}">

TODO: add note about what to do when the JS call specifies the location (just need to change the CSS classes…)

I changed and tested property-click, hier-trigger. For redirected-vocab-id I changed, but didn't know how to test it. Also, redirected-vocab-id had three occurrences, but only 1 with a title attribute (used by qtip and by this pure-css approach (so I am assuming the other two can be ignored).

I couldn't load AGROVOC to test reified properties. @osma would you have another smaller example dataset that I can load to test it, please?

The reified icon is trickier, I just realized, as it displays not a text element, but instead a child div with other HTML elements. Gotta have to think more how to replace this one with pure css (still doable I think).

Thanks
-Bruno

I couldn't load AGROVOC to test reified properties. @osma would you have another smaller example dataset that I can load to test it, please?

Can you use the snippet from #1356 (comment) ? See the screenshots in the same comment for how it should look (but note that the display of that kind of partial SKOS XL data is a bit broken unless you also apply the changes in PR #1356)

PR #1356 was just merged to master. I suggest you merge the changes from master to this branch, then it's easier to test the tooltips for SKOS XL properties.

Rebased, and then modified my existing YSO data to have the prefix for skosxl and to have the property in the p6095 concept as in the example you provided @osma.

But now if I open p6095 it's giving me a blank page and the Docker logs contain:

skosmos-web     | [Tue Sep 13 06:44:34.769517 2022] [php7:error] [pid 11] [client 172.19.0.1:41434] PHP Fatal error:  Uncaught EasyRdf\\Http\\Exception: HTTP request for SPARQL query failed in /var/www/html/vendor/easyrdf/easyrdf/lib/Sparql/Client.php:247\nStack trace:\n#0 /var/www/html/vendor/easyrdf/easyrdf/lib/Sparql/Client.php(128): EasyRdf\\Sparql\\Client->request()\n#1 /var/www/html/model/sparql/GenericSparql.php(93): EasyRdf\\Sparql\\Client->query()\n#2 /var/www/html/model/sparql/GenericSparql.php(1475): GenericSparql->query()\n#3 /var/www/html/model/Concept.php(536): GenericSparql->queryLabel()\n#4 /var/www/html/vendor/twig/twig/src/Extension/CoreExtension.php(1566): Concept->getProperties()\n#5 /tmp/skosmos-template-cache/ed/edd3a5fbdba4b947d8c145cd98bb65cab172eaea5513f6ffe050cafc75630ff2.php(400): twig_get_attribute()\n#6 /var/www/html/vendor/twig/twig/src/Template.php(405): __TwigTemplate_34d06a17ee23cf00128016f7cd55bc42f28d9e449367abf9e5f1bad707bb4a00->doDisplay()\n#7 /var/www/html/vendor/twig/twig/src/Template.php(378): Twig\\Template->displayWithErrorHandling()\n#8 /tmp/skosmos-template-cache/66/66e752b362c577ad7cde0210772fed34 in /var/www/html/view/concept-shared.twig on line 95

Do you know if I need to modify something else in my Skosmos installation to have skos-xl now? I can access other concepts just fine, it's just this one that's giving me the error.

Thanks

That looks like a SPARQL query failed. Can you do either/both of these:

1. enable SPARQL query logging in config.ttl to a file so you can see the query that failed
2. check the Fuseki log

Rebased.

Looks similar enough, and the font is actually now better, the old version was off. The only semi-important regression is that in the second paragraph, there is some extra space before the first word (the same happens with text in other languages). Pretty minor, but if there's an easy fix, it could be removed.

Fixed! I used pre-wrap which breaks at newline symbols, but keeping white spaces. Now I've replaced it by pre-line, which breaks at newlines but collapses white spaces.

However, if I actually click on "Help", this happens:

The language selection pops up in front of the tooltip, just like the cross icon did earlier. Some z-index adjustment needed?

Huh, that's very interesting! There was a :focus rule, that sets any element with focus to z-index: 1 in styles.css. Fixed that by specifying the same z-index for the tooltip and for the toolip when :focus is present/active.

Reified definitions
This is an improvement! For some reason the color was off in the old version. I also like the new layout better. But the text size is a bit larger than before.

I checked that, and it's using the root css variable, set for 14px, like the other tooltips, @osma. I think in the past the reified was also using 14px, but maybe the other elements were using a larger font? Not sure.

Would you like me to make just the reified tooltips a little smaller? Maybe 12px?

Here's a preview where I didn't touch the CSS of the normal tooltip, used in the Help link, and where I reduced the reified tooltip text to 12px:

Here is also a bug, the label for the property dc11:source is not being shown properly (should be "Source"). But it's not caused by this PR. I know the cause (I already fixed a similar case with SKOS XL labels), I fill fix it in a separate PR.

👍 Thanks!

Other
There's an unintended change to the property names.
Previously, only "TYPE" was set in bold. Now the other property labels are in bold as well, except for "IN OTHER LANGUAGES" which is a bit special as it's not a regular SKOS/RDF property.

Ah! Good spot, I accidentally removed spaces between css classes, so it was using versal-boldpropertyclick or something similar to that, instead of versal-bold propertyclick. It should be fixed now.

SKOS XL information for the prefLabel as a heading
SKOS XL information for the prefLabel in another language

I think this is the last pending item in your review, @osma. I will need a little more time to re-read it and understand how/what I should reproduce and test it. I think it's with the birches example I copied into my copy of YSO vocab, but it appears to be working correctly for me, but also not rendering like in your screenshots. Will leave this one to be fixed for over the next days 👋

Thanks!

Thanks again @kinow!

I made a new round of testing, this time without applying the Finto CSS layout in case it might interfer. It looks great now, pretty much all of my worries have been addressed. I could only find two quite minor and trivial things to change:

Property name tooltips

This is something I forgot to look at in detail in my previous review.

Before:

After:

1. Is it possible to show a little arrowhead pointing at the property name, to make it clear what the tooltip is referring to?
2. The text is all capitalized even though it shouldn't be.

Display differences between reified definitions and SKOS XL labels

Here is a tooltip for a reified definition resource:

Here is a similar tooltip for a SKOS XL label:

They are very similar but not identical. The property name "Source:" is set in bold in the SKOS XL tooltip, but not the one for the definition. I like the bold version more, so could the tooltip for definitions (and similar resources) also use bold face for the property name?

Other than these, I can't think of anything more that would need to be fixed before merging, so I think this is really, really close now! And already this is IMHO better than the current status quo because qtip2 seems to be a bit buggy (sometimes tooltip contents just disappear) and also there are some font and color issues with the old tooltips that this PR fixes as well 👍

Other than these, I can't think of anything more that would need to be fixed before merging, so I think this is really, really close now! And already this is IMHO better than the current status quo because qtip2 seems to be a bit buggy (sometimes tooltip contents just disappear) and also there are some font and color issues with the old tooltips that this PR fixes as well +1

Thanks a lot for testing it again and for the great feedback and screenshots @osma. I think we will have it ready for a final review and merge in the next iteration (i.e. next commits pushed soon). All your points sound doable, and not too hard 🙂

Replies inline

Property name tooltips

1. Is it possible to show a little arrowhead pointing at the property name, to make it clear what the tooltip is referring to?
2. The text is all capitalized even though it shouldn't be.

Done! I also changed the alignment of the tooltip for t-top class. They should now appear left-aligned, more similar to what it was before. In the screenshot below I force two tooltips to be displayed, which is not possible normally, but you can force with the browser development tools. That's just to show that the two tooltips are now left-aligned, which I think makes it easier to keep your eyes in place as you move them down following the mouse to read the tooltips (easily reverted if needed). Also added the arrow sticking out of the tooltip balloon. It was covered by the tooltip.

Display differences between reified definitions and SKOS XL labels

[x] They are very similar but not identical. The property name "Source:" is set in bold in the SKOS XL tooltip, but not the one for the definition. I like the bold version more, so could the tooltip for definitions (and similar resources) also use bold face for the property name?

These are controlled in the template, not in the CSS style. I'm looking at the birches concept page. The tooltip of the Definition icon doesn't have bold anywhere. The tooltip of the Text In Other Languages does.

That's controlled by having the versal-pref at the top element (with text-weight: 500), and the versal class at the tooltip text after the “Source” text (with text-weight: 400). I changed the template to have the Definition icon tooltip to use a similar structure, but note that that activates the a.versal class, which makes the text blue (I think you mentioned you liked more the all white text, easy to change it if needed, but not sure if that a.versal is not used elsewhere). Preview:

After this screenshot, I re-read your comment, @osma, and after reading this part “also use bold face for the property name?”, so I moved the versal-pref up one level. Could you take a look if that's what you meant? Otherwise we just need to move the CSS class one level down and we will have the result of the previous screenshot. Preview:

I also noticed that to reach the link sometimes the tooltip would disappear if the mouse cursor accidentally hovered outside the tooltip. I tweaked a bit the CSS classes and the location of the tooltip arrow, to see if that reduces the chances of the tooltip disappearing while the user moves it to click on the Source link. qtip could use JS to disappear with the tooltip after a few milliseconds, which gave some buffer for the user to move the mouse outside the element. We don't have that feature with CSS-only (a good trade-off I think, given that it reduces the JS dependencies and simplifies the code a bit IMO).

The good news is that if there are other parts where the tooltip must adjust the text weight we should be able to do so without changing any or much of the tooltip CSS, and controlling that via the existing CSS classes and HTML structure.

Let me know if this is looking better or if we need to further adjust it (feel free to commit directly if you'd like, I can sync the branch and post a preview screenshot here too). So ready again for review whenever you have time @osma! Thanks 👍

Cheers,
Bruno

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
193 Code Smells

No Coverage information
1.1% Duplication

Thanks @kinow ! Everything was good except I wasn't 100% happy with the way properties and values were displayed - which was inconsistent from the beginning so no fault in this PR really). I made a few more tweaks in a commit that I just pushed to your PR branch. I will merge this now 🎉

Thanks @osma!

A rather strange artifact that looks like an empty tooltip box has appeared to dev.finto.fi where we are testing the new skosmos version. This appears when mouse is hovered over "Hierarchy" tab while any of the other tabs are active. See for example: https://dev.finto.fi/yso/en/

According to @osma this might be related to this PR, any ideas about the cause and possible fix Osma and @kinow ?

Hi @MikkoAleksanteri

A rather strange artifact that looks like an empty tooltip box has appeared to dev.finto.fi where we are testing the new skosmos version. This appears when mouse is hovered over "Hierarchy" tab while any of the other tabs are active. See for example: https://dev.finto.fi/yso/en/

Ah, I loved the description 😄 That's the tooltip for the Hierarchy tab option I think. The rotated (45 degree?) square is the "triangle" or "arrow" that sticks out of the tooltip. The black/darker part is the actual tooltip, but it appears to be completely empty. Causing it to look strange that way.

According to @osma this might be related to this PR, any ideas about the cause and possible fix Osma and @kinow ?

That's really odd. I couldn't see anything wrong from a quick inspection with the browser. The top menu “Help” option has a tooltip too, and that one appears to be working fine. Let me check the templates to see if there's anything that could break it, and if I cannot find it I will try to reproduce it locally.

Thanks @MikkoAleksanteri
-Bruno

Alright, looking at the template file light.twig, looks like it is rendering the tooltip for disabled hierarchy. But the template hides the message when hierarchy is not disabled. We need to disable the tooltip as well. My bad, let me see how to fix this one. Thanks for reporting!

Reproduced with YSO locally using master:

Thank you @kinow for the quick response and fix!